TYP: annotate functions that always error with NoReturn #48002
Conversation
| @@ -1243,7 +1244,7 @@ def groupings(self) -> list[grouper.Grouping]: | |||
| ping = grouper.Grouping(lev, lev, in_axis=False, level=None) | |||
| return [ping] | |||
|
|
|||
| def _aggregate_series_fast(self, obj: Series, func: Callable) -> np.ndarray: | |||
| def _aggregate_series_fast(self, obj: Series, func: Callable) -> NoReturn: | |||
There was a problem hiding this comment.
When we have NotImplementedError, does the NoReturn not create issues with subtypes? (in general, no checked this particular case)
There was a problem hiding this comment.
Yes it would create issues with sub-types that actually implement these functions. Parent methods should probably be abstract with a signature that the child will implement.
It should be safe adding NoReturn to final methods as no child class is meant to implement them. Based on the doc-string of _aggregate_series_fast I assume this method is basically deprecated and is not implemented in sub-classes.
There was a problem hiding this comment.
Thanks. yes it does appear that the exception is not for public consumption.
There was a problem hiding this comment.
NoReturn is useful in cases like this to narrow the type:
def error() -> NoReturn: ...
def test(x: int | str) -> str:
# x: int | str
if isinstance(x, int):
error()
# x: str
return x + "some_string"I don't there are many places where NoReturn can be used in pandas, in most cases we should make classes abstract.
Is there a historic reason to avoid inheriting from ABC and using abstractmethod? Happy to make PRs to make more methods abstract.
There was a problem hiding this comment.
Thanks @twoertwein lgtm apart from general question on generic usage of NoReturn
|
Thanks @twoertwein |
There are more non-abstract functions that also error. Most of them should probably be abstract functions. I ignored those functions.